Skip to content

Conversation

alextrnnn
Copy link
Contributor

Changes in this pull request

Give a narrative description of what has been changed.

Checklist

  • This PR represents a single feature, fix, or change.
  • All applicable changes have been documented.
  • Any TO DO items (or similar) have been entered as GitHub issues and the link to that issue has been included in a comment.

@alextrnnn alextrnnn changed the title Alextrnnn/multi asset hash feat: multi asset hash Aug 11, 2025
@alextrnnn alextrnnn marked this pull request as ready for review August 13, 2025 15:44
Copy link
Contributor

@ok-nick ok-nick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Just a few thoughts that we will have to figure out how to address.

/// See <https://spec.c2pa.org/specifications/specifications/2.2/specs/C2PA_Specification.html#_multi_asset_hash>
#[derive(Deserialize, Serialize, Debug, PartialEq)]
pub struct MultiAssetHash {
pub parts: Vec<PartHashMap>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add docs for these fields from the spec.

Comment on lines +68 to +72
// Keep track of the size of optional parts.
if part.optional.unwrap_or(false) {
optional_sizes += locator.length;
}
expected_offset += locator.length;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also verify the byte offsets so that each part's length + byte offset doesn't overlap and also doesn't extend past the end of the file.

claim: &Claim,
asset_handler: Option<&dyn AssetIO>,
) -> Result<()> {
let length = stream_len(reader)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to look into this for other parts of the SDK, there is mixed usage of this and .stream_position().

Comment on lines +146 to +151
// Read only the specified parts within the larger stream.
reader.seek(std::io::SeekFrom::Start(offset))?;
let buf = reader.read_to_vec(length).map_err(|_| {
Error::C2PAValidation(ASSERTION_MULTI_ASSET_HASH_MISSING_PART.to_string())
})?;
let mut part_reader = Cursor::new(buf);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be able to use Read::take here instead of allocating a vec so we keep it purely streams.

}

// Extract XMP from bytes.
fn extract_xmp_extensions(seg: &JpegSegment) -> Option<XmpExtension> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work here.

@alextrnnn
Copy link
Contributor Author

Sorry for the delay and thanks for the feedback @ok-nick ! I'll try to address these over the weekend

Copy link

codspeed-hq bot commented Sep 5, 2025

CodSpeed Performance Report

Merging #1296 will not alter performance

Comparing alextrnnn/multi-asset-hash (0f3e90d) with main (74bb71d)

Summary

✅ 18 untouched benchmarks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants